Rails7 fixes #1263

jrafanie · 2024-07-09T20:41:29Z

These are backwards compatible changes to get rails 7 to work on this repository.

Cross repo test with rails 7 core branch

jrafanie · 2024-07-10T20:54:36Z

app/controllers/api/base_controller/normalizer.rb

@@ -56,6 +56,10 @@ def normalize_attr(attr, value)
          Float::INFINITY.to_s
        elsif Api.resource_attribute?(attr)
          normalize_resource(value)
+        elsif value == NilClass
+          {}


@kbrock do you have any idea why this was needed? The test below was expecting a NilClass v_workflow_class to come back as {}. Not a string, not json null, but {}. Note, it's the NilClass object, a Class Object, not nil (an instance of NilClass).

jrafanie · 2024-07-10T20:57:48Z

spec/requests/requests_spec.rb

@@ -269,8 +269,7 @@
        "id"               => request.id.to_s,
        "workflow"         => a_hash_including("values"),
        "v_allowed_tags"   => [a_hash_including("children")],
-        "v_workflow_class" => a_hash_including(
-          "instance_logger" => a_hash_including("klass" => request.workflow.class.to_s))


I have no idea why this passed on rails 6.1 🤣

FYI, instance_logger is an instance variable coming from the Vmdb::Logging stuff. Not sure why this was coming back previously. Maybe json dump of the class object included the ivars. That's why I'm changing it to return the Class#name as it's weird to store the Class object.

where is v_workflow_class defined?

https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_request.rb#L51

The rescue nil in the class method drives me mad and I think it's related to the NilClass {} above. I really have no idea how any of this stuff works but this instance_logger in the test was completely wrong and thankfully rails 7 detected it. Not sure why it worked in 6.1

Wow, I had so much trouble tracking down where this method was defined delegate is not very grep friendly. It was added in ManageIQ/manageiq#13441

virtual_column :v_workflow_class, :type => :string, :uses => :workflow delegate :class, :to => :workflow, :prefix => :v_workflow

Interestingly, there is a workflow_class method that we probably should be using instead.
But more importantly, it is declared to return a String (which it probably should be), but it returns a Class. It is possible that our serialization of a Class may have changed.

It appears that the example is not properly creating a workflow, so nil.class is returning a NilClass.

I put a byebug into the class but getting all sorts of errors:

These all work in rails c for me, but fail when I'm running this specific spec.

(byebug) MiqRequest.new.description *** NameError Exception: uninitialized constant MiqRequest::TASK_DESCRIPTION default_value_for(:message) { |r| "#{r.class::TASK_DESCRIPTION} - Request Created" } (byebug) request.class MiqProvisionRequest (byebug) MiqProvisionRequest.new.description nil (byebug) request.v_workflow_class *** NameError Exception: uninitialized constant MiqRequestWorkflow::DialogFieldValidation include DialogFieldValidation ^^^^^^^^^^^^^^^^^^^^^ (byebug) require "miq_request_workflow/dialog_field_validation" (byebug) MiqProvisionRequest.new.v_workflow_class (byebug) MiqProvisionRequest.new.workflow_class ^^^ all give the same error

I tried redefining the DialogFieldValidation to the long style (class MiqProvisionWorkflow; module DialogFieldValidator; end; end instead of module parent::DialogFieldValidator) but that did not help.

This is a tangent, but this feels a little chicken and egg.
If you want to define the child module, define the parent class (which references the child module)

update:

The MiqProvisionRequest.workflow_class references a class, but constantize throws that exception listed above. Match that with a rescue nil and we get a nil coming out. nil.class == NilClass

yeah, is the test and code busted or just the test? I can try to create a proper workflow to fix the test.

Thnx Jason: once I updated all my repos, I was able to load this just fine.

One thing about the code, it compares the workflow.class and the workflow_class. So if it gets busted (goes from a value to nil), then it will always pass the test.

I'm punting on this (see below). We'll tackle this if we determine the correct behavior it should have. Right now, it doesn't make sense why it's ok on rails 6.1 when it's clearly wrong. It's possible no one is using it and not reported a bug. Either way, we can handle it when it's a problem.

jrafanie · 2024-07-10T21:00:06Z

spec/requests/servers_spec.rb

@@ -110,7 +110,7 @@
      server = FactoryBot.create(:miq_server, :status => 'started')

      expect { post(api_server_url(nil, server), :params => gen_request(:delete)) }.to change(MiqServer, :count).by(0)
-      expect_single_action_result(:success => false, :message => 'Failed to destroy the record')
+      expect_single_action_result(:success => false, :message => 'Failed to destroy')


Rails change the method here to add more record information so it's easier to use a simpler string check to satisfy this on rails 6.1 and 7.

kbrock · 2024-07-17T04:04:52Z

While I think we do want to understand why NilClass is not working the same as all other classes, and previous rails NilClass behavior, let us remember that this method under test is bogus and needs to be removed.

It is not referenced (don't think it ever has been), and I find it hard to believe that we want a method that returns an obscure hash. My bet is we wanted the name of the workflow class. TBH/ not sure why the front end would even need that.

jrafanie · 2024-07-23T17:16:34Z

While I think we do want to understand why NilClass is not working the same as all other classes, and previous rails NilClass behavior, let us remember that this method under test is bogus and needs to be removed.

It is not referenced (don't think it ever has been), and I find it hard to believe that we want a method that returns an obscure hash. My bet is we wanted the name of the workflow class. TBH/ not sure why the front end would even need that.

Ok, I'll drop the components of the tests that are checking v_workflow_class. I agree that we probably want the class name... so I think the first hunk in the patch below seems like the correct thing to do... but I'm fine with doing it if we need to do it.

diff --git a/app/controllers/api/base_controller/normalizer.rb b/app/controllers/api/base_controller/normalizer.rb
index f2585e2c..ddefee7d 100644
--- a/app/controllers/api/base_controller/normalizer.rb
+++ b/app/controllers/api/base_controller/normalizer.rb
@@ -56,6 +56,10 @@ module Api
           Float::INFINITY.to_s
         elsif Api.resource_attribute?(attr)
           normalize_resource(value)
+        elsif value == NilClass
+          {}
+        elsif value.kind_of?(Class)
+          value.name
         else
           value
         end
diff --git a/spec/requests/requests_spec.rb b/spec/requests/requests_spec.rb
index f9a20d49..9111cc7c 100644
--- a/spec/requests/requests_spec.rb
+++ b/spec/requests/requests_spec.rb
@@ -269,8 +269,7 @@ RSpec.describe "Requests API" do
         "id"               => request.id.to_s,
         "workflow"         => a_hash_including("values"),
         "v_allowed_tags"   => [a_hash_including("children")],
-        "v_workflow_class" => a_hash_including(
-          "instance_logger" => a_hash_including("klass" => request.workflow.class.to_s))
+        "v_workflow_class" => request.workflow.class.to_s
       )

       expect(response.parsed_body).to match(expected_response)
@@ -318,7 +317,7 @@ RSpec.describe "Requests API" do

       expected_response = a_hash_including(
         "id"               => request.id.to_s,
-        "v_workflow_class" => {}
+        "v_workflow_class" => {} # Need to figure out why NilClass comes back as {} previously
       )

       expect(response.parsed_body).to match(expected_response)

jrafanie · 2024-07-23T17:19:56Z

@miq-bot cross-repo-tests ManageIQ/manageiq#22873

From Pull Request: ManageIQ/manageiq-api#1263

miq-bot · 2024-07-23T17:35:34Z

Checked commits jrafanie/manageiq-api@2fb626a~...3ebce41 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

jrafanie · 2024-07-23T17:56:20Z

Ok, I think I resolved all the issues. I've removed the bogus test areas that asserted values for v_workflow_class as that's not functioning "correctly" and we don't have good reasons to change the bogus behavior, so we'll tackle that later if needed.

kbrock · 2024-07-23T22:09:57Z

Glad you dropped this rather than trying to track down our serialization of classes (which was probably a non-implemented feature anyway)

Rails 7 added details to the message, so simplify the check

2fb626a

jrafanie added the rails7 label Jul 9, 2024

jrafanie requested a review from bdunne as a code owner July 9, 2024 20:41

jrafanie added the wip label Jul 9, 2024

jrafanie mentioned this pull request Jul 9, 2024

Upgrade to rails 7.0 ManageIQ/manageiq#22052

Closed

jrafanie commented Jul 10, 2024

View reviewed changes

jrafanie force-pushed the rails7_fixes branch from 7bffe1b to 36f7595 Compare July 23, 2024 17:17

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jul 23, 2024

Running tests for jrafanie

417632d

From Pull Request: ManageIQ/manageiq-api#1263

miq-bot mentioned this pull request Jul 23, 2024

[BOT] Cross repo test for ManageIQ/manageiq-api#1263 ManageIQ/manageiq-cross_repo-tests#883

Open

jrafanie force-pushed the rails7_fixes branch from 36f7595 to 25c60e4 Compare July 23, 2024 17:28

Remove testing of v_workflow_class

3ebce41

jrafanie force-pushed the rails7_fixes branch from 25c60e4 to 3ebce41 Compare July 23, 2024 17:31

jrafanie removed the wip label Jul 23, 2024

jrafanie changed the title ~~[WIP] Rails7 fixes~~ Rails7 fixes Jul 23, 2024

kbrock self-assigned this Jul 23, 2024

kbrock added the enhancement label Jul 23, 2024

kbrock merged commit 20fd786 into ManageIQ:master Jul 23, 2024
4 checks passed

jrafanie deleted the rails7_fixes branch July 25, 2024 14:23

kbrock added test and removed enhancement labels Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails7 fixes #1263

Rails7 fixes #1263

jrafanie commented Jul 9, 2024 •

edited

Loading

jrafanie Jul 10, 2024 •

edited

Loading

jrafanie Jul 10, 2024

jrafanie Jul 10, 2024

Fryguy Jul 11, 2024

jrafanie Jul 11, 2024

jrafanie Jul 11, 2024

kbrock Jul 15, 2024 •

edited

Loading

kbrock Jul 15, 2024

jrafanie Jul 15, 2024

kbrock Jul 17, 2024

jrafanie Jul 23, 2024

jrafanie Jul 10, 2024

kbrock commented Jul 17, 2024

jrafanie commented Jul 23, 2024

jrafanie commented Jul 23, 2024

miq-bot commented Jul 23, 2024

jrafanie commented Jul 23, 2024

kbrock commented Jul 23, 2024

Rails7 fixes #1263

Rails7 fixes #1263

Conversation

jrafanie commented Jul 9, 2024 • edited Loading

jrafanie Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Jul 17, 2024

jrafanie commented Jul 23, 2024

jrafanie commented Jul 23, 2024

miq-bot commented Jul 23, 2024

jrafanie commented Jul 23, 2024

kbrock commented Jul 23, 2024

jrafanie commented Jul 9, 2024 •

edited

Loading

jrafanie Jul 10, 2024 •

edited

Loading

kbrock Jul 15, 2024 •

edited

Loading